Skip to content

test(api,trpc): replace module mocks with stubs scoped to one file - #8315

Merged
saddlepaddle merged 1 commit into
mainfrom
test/api-trpc-no-module-mocks
Oct 8, 2026
Merged

saddlepaddle merged 1 commit into
mainfrom
test/api-trpc-no-module-mocks

Conversation

@saddlepaddle

@saddlepaddle saddlepaddle commented Oct 8, 2026 •

Copy link
Copy Markdown
Collaborator

Part of removing cross-file test leaks so suites do not depend on bun test --isolate. mock.module is process-wide in bun and has no undo: without --isolate, a fake reaches every test file that loads later.

What changed

All 59 mock.module calls in apps/api tests and 24 in packages/trpc tests are gone. No production code changed, so no tRPC contract changed.

  • Scoped stubs. test/stub.ts exports stub(target, fakes): it swaps members of a live object or module namespace in beforeAll (with spyOn for functions) and restores them in afterAll. Exports that a file does not list stay real, so the "list every export" partial fakes are gone.
  • Env from a preload. apps/api had no test preload. It now has bunfig.toml and test/setup-env.ts with placeholders and the fixed values the tests assert on, which removes the six @/env mocks.
  • Third-party classes are stubbed on their prototypes; run-agent.test.ts throws the real Anthropic error classes.

This is the same amount of faking as before, scoped to one file. It does not inject dependencies: passing db through every router and route handler is a production change this PR does not make.

Leaks fixed

Leaker Victim Cause
linear/webhook/route.test.ts processDelivery.test.ts partial fake of ./processDelivery
every file that mocked @superset/db/client recordWebhookDelivery.test.ts and others one file's fake db reached later files
plugins/proxy/resolve-target.test.ts connectors/refresh.test.ts whole-module fakes of lookup, refresh, upsert
slack/events/route.test.ts (in-file) itself an unused mockImplementationOnce carried into the next test
plugins/proxy/plugin-server.test.ts (in-file) itself the server caches account layouts by plugin version and every test used the same one
root .env sandbox/credentials.test.ts the test assumed NEXT_PUBLIC_API_URL was unset

No test was deleted. api runs 230 tests (some files used to fail at load without --isolate), trpc 570.

Known limits

  • test/stub.ts is two identical files, one per package.
  • public-access.test.ts used to trap every access to db; it now traps query, select, insert, update, delete, execute and transaction.
  • Three *.integration.ts files in packages/trpc still call mock.module("../../lib/r2"). bun test does not match them.

Verification

bun 1.4.2, bun test --randomize --seed N, seeds 1-12, in both packages: all pass without --isolate and with it. Also run with the root .env neutralised, to match CI. Repo typecheck and biome pass.


Summary by cubic

Replaces bun's process-wide and irreversible mock.module with per-file stubs so test suites no longer depend on bun test --isolate. A mock.module factory in one test file leaked to every test file loaded later and could not be undone; the new stub helper swaps members of the live module or object in beforeAll and restores them in afterAll. No production code or tRPC contract changed, and no tests were deleted (api: 230, trpc: 570).

Changes

  • Added test/stub.ts per package; partial "list every export" fakes are gone because exports a file does not stub stay real.
  • apps/api gained a bunfig.toml preload and test/setup-env.ts with fixed env values, removing the six @/env mocks.
  • Third-party classes are stubbed on their prototypes; run-agent.test.ts now throws real Anthropic error classes.
  • Fixed in-file leaks: a leftover mockImplementationOnce in slack/events/route.test.ts and cached account layouts by plugin version in plugin-server.test.ts.

Known limits

  • test/stub.ts is duplicated across the two packages.
  • public-access.test.ts traps a subset of db operations instead of every property access.
  • Three *.integration.ts files in packages/trpc still call mock.module; bun test does not match them.

Verified with bun test --randomize --seed 1..12 in both packages, with and without --isolate, and with the root .env neutralized; repo typecheck and biome pass.

Written for commit f82a83b. Summary will update on new commits.

View guided diff Turn on auto-fix

Summary by CodeRabbit

  • Tests
    • Updated API and workspace tests to use more targeted, automatically restored test doubles instead of process-wide module overrides.
    • Standardized test environment setup with fallback configuration values and support for temporarily clearing environment variables.
    • Adjusted test fixtures and configuration to improve consistency across test suites.

mock.module is process-wide in bun and has no undo. Without --isolate, a
partial fake of ./processDelivery, @superset/db/client and others reached
later test files (processDelivery, recordWebhookDelivery and connector
refresh tests failed, depending on order).

Each file now swaps members of the real module or object in beforeAll and
restores them in afterAll. apps/api gets a test preload with fixed env
values, so the @/env mocks are gone. No production code changes.
@coderabbitai

coderabbitai Bot commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: fe9eac8a-c65d-4531-889c-12b4258a3db2
📥 Commits

Reviewing files that changed from the base of the PR and between 3fb943b and f82a83b.

📒 Files selected for processing (32)
  • apps/api/bunfig.toml
  • apps/api/src/app/api/account/jobs/purge-expired-deletions/route.test.ts
  • apps/api/src/app/api/integrations/linear/webhook/processDelivery.test.ts
  • apps/api/src/app/api/integrations/linear/webhook/route.test.ts
  • apps/api/src/app/api/integrations/slack/events/process-agent-message/process-agent-message.test.ts
  • apps/api/src/app/api/integrations/slack/events/route.test.ts
  • apps/api/src/app/api/integrations/slack/events/utils/run-agent/run-agent.test.ts
  • apps/api/src/app/api/integrations/slack/events/utils/thread-sessions/thread-sessions.test.ts
  • apps/api/src/app/api/integrations/slack/interactions/route.test.ts
  • apps/api/src/app/api/integrations/slack/jobs/process-mention/route.test.ts
  • apps/api/src/lib/ingest/recordWebhookDelivery.test.ts
  • apps/api/src/lib/integrations/oauthFlow.test.ts
  • apps/api/src/lib/integrations/resolveCallback.test.ts
  • apps/api/src/lib/oauth-state.test.ts
  • apps/api/test/setup-env.ts
  • apps/api/test/stub.ts
  • apps/api/tsconfig.json
  • packages/trpc/src/lib/account-purge/findOrganizationSolelyOwnedBy.test.ts
  • packages/trpc/src/lib/account-purge/purgeAccount.test.ts
  • packages/trpc/src/lib/cloud-guards/cloud-guards.test.ts
  • packages/trpc/src/lib/connectors/refresh.test.ts
  • packages/trpc/src/lib/sandbox/credentials.test.ts
  • packages/trpc/src/router/cloud-workspace/reap.test.ts
  • packages/trpc/src/router/page-comment/public-access.test.ts
  • packages/trpc/src/router/plugins/plugins.test.ts
  • packages/trpc/src/router/plugins/proxy/plugin-server.test.ts
  • packages/trpc/src/router/plugins/proxy/resolve-target.test.ts
  • packages/trpc/src/router/user/user.test.ts
  • packages/trpc/src/trpc.test.ts
  • packages/trpc/test/env.ts
  • packages/trpc/test/setup-env.ts
  • packages/trpc/test/stub.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 4 remain after this review.


📝 Walkthrough

Walkthrough

The pull request adds test environment setup and reusable stubbing helpers for the API and tRPC packages. It converts API and tRPC tests from process-wide module mocks to stubs on imported modules while retaining the described fixture behavior.

Changes

Test setup and stubbing

Layer / File(s) Summary
Test environment and stubbing helpers
apps/api/bunfig.toml, apps/api/test/*, apps/api/tsconfig.json, packages/trpc/test/*
Adds API test preload and environment configuration, plus stub helpers in both packages that restore spies and properties after tests. The tRPC environment helper accepts undefined to remove environment variables.
API test stubbing
apps/api/src/app/api/account/jobs/..., apps/api/src/app/api/integrations/linear/webhook/*, apps/api/src/app/api/integrations/slack/events/..., apps/api/src/app/api/integrations/slack/interactions/route.test.ts, apps/api/src/app/api/integrations/slack/jobs/..., apps/api/src/lib/ingest/recordWebhookDelivery.test.ts, apps/api/src/lib/integrations/*.test.ts, apps/api/src/lib/oauth-state.test.ts
Replaces module-level mocks with direct stubs across API tests. The described database, service, and SDK fixture behavior remains; retry tests use Anthropic SDK error instances, and the Slack route resets shared mocks in beforeEach.
tRPC test stubbing
packages/trpc/src/lib/account-purge/*, packages/trpc/src/lib/cloud-guards/cloud-guards.test.ts, packages/trpc/src/lib/connectors/refresh.test.ts, packages/trpc/src/lib/sandbox/credentials.test.ts, packages/trpc/src/router/cloud-workspace/reap.test.ts, packages/trpc/src/router/page-comment/public-access.test.ts, packages/trpc/src/router/plugins/*, packages/trpc/src/router/user/user.test.ts, packages/trpc/src/trpc.test.ts
Replaces module-level mocks with direct stubs on imported modules. The plugin-server test assigns a distinct version to each target, and the credentials test sets NEXT_PUBLIC_API_URL to undefined.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Other

Suggested reviewers: harshithmullapudi

Merge Risk: ⚪ Minimal · up to f82a8

No confirmed issue blocks merging after normal checks. The credential test’s environment timing remains unverified.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 57.14% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 30 files. (2 skipped: … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title uses conventional commit format and clearly describes the main change: replacing process-wide module mocks with file-scoped stubs.
Description check ✅ Passed The description explains the motivation, implementation, known limits, and test verification. It does not include the template checklist or explicitly confirm the maintainers-edit setting, but it prov…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

Docstring coverage is 57.14% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 30 files. (2 skipped: 2 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

🧹 Preview Cleanup Complete

The following preview resources have been cleaned up:

  • ✅ Neon database branch

Thank you for your contribution! 🎉

@expo

expo Bot commented Oct 8, 2026

Copy link
Copy Markdown

Preview update published for development build 512c3db4 (fingerprint 37d39e6ebc3d). Open the build on your phone and scan:

Update group 291a34d6

@greptile-apps

greptile-apps Bot commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

[Low risk] Test infrastructure refactored to use scoped stubs instead of global mocks.

This PR appears safe to merge; no actionable issues were found.

What we checked:

  • Tests reuse another layout: Each target gets a different version. The layout cache includes that version in its key.
  • Delivery test calls real database: recordWebhookDelivery imports and calls db, whose execute member remains stubbed.

Summary

Replaces process-wide test module mocks with scoped member stubs in the API and tRPC packages. Production code and tRPC contracts are unchanged.

  • Adds preload values so tests can import real clients.
  • Clears unused Slack mock responses between tests.
  • Gives plugin test targets separate cache keys.
  • No actionable issues were found. Tests were not run during this review.

Known limits acknowledged by saddlepaddle: duplicate stub helpers, a narrower database trap in public-access.test.ts, and module mocks remaining in three integration files.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Test file loads] --> B[Register stub hooks]
  B --> C[beforeAll swaps members]
  C --> D[Tests use fakes]
  D --> E[afterAll restores members]
  E --> F[Later files use real members]
Loading

Reviews (1) · Last reviewed commit: "test(api,trpc): replace module mocks wit..." · Reviewed by Greptile

@saddlepaddle
saddlepaddle merged commit 0a26a30 into main Oct 8, 2026
20 checks passed
@saddlepaddle
saddlepaddle deleted the test/api-trpc-no-module-mocks branch October 8, 2026 05:43
saddlepaddle added a commit that referenced this pull request Oct 8, 2026
* test: run suites without --isolate and remove three flaky tests

The cross-file leaks that --isolate hid are fixed (#8314, #8315, #8316,
#8319), so each package's suite now runs in one process. The desktop suite
takes about 38 s without the flag and about 73 s with it.

apps/relay still had one leak: host-tunnel.test.ts replaced partyserver
with a stub that has no getServerByName, which broke index.test.ts in some
file orders. The test passes against the real module, so the stub is gone.

Removed as flaky:
- workspace-fs: FsWatcherManager resumes a subscription after the root is
  deleted and recreated
- i18n: locale changes preserve state and focus (provider-state)
- agent-setup: Pi completion notifications, which poll up to 1 s for a
  detached script

* test(trpc): keep the opt-in Postgres watch test out of the shared suite

It sets DATABASE_URL and then imports the db client. In one process the
client may already exist, so the test would write its fixtures to the
database the first file connected to. As an .integration.ts file it runs
only when named, in its own process, like the other opt-in suites.

This branch was previously deployed

1 inactive deployment
preview — f82a83b9 Deployed Oct 8, 2026 by saddlepaddle via Deploy Docs #11511
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant